-
-
Notifications
You must be signed in to change notification settings - Fork 286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework config and reload config on file change/creation/deletion #663
Conversation
also introduces the utils folder
@@ -84,16 +84,6 @@ def file_stats(): | |||
auto_cpufreq_stats_file = open(auto_cpufreq_stats_path, "w") | |||
sys.stdout = auto_cpufreq_stats_file | |||
|
|||
def get_config(config_file=""): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that this is gone, I fail to understand where the "config" file is picked up with _Config class and pyinotify. Could you please point me to line where this is happening?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the config instance is imported from the config.py
file and you can access the config by calling config.get_config()
. This is seen in core.py
in all of the set functions (where just get_config()
) used to be
Good stuff with a lot of potential! Good thing is that it when writing to auto-cpufreq file, changes are immediately reflected and picked up. However if I do Also, after auto-cpufreq-installer auto-cpufreq GUI wasn't there, nor was it available after
and same thing was after remove, i.e:
Besides this, I also left couple of minor comments in code. |
Fixed with 6cf4a66 by adding the IN_MOVED_TO and IN_MOVED_FROM events
All fixed in 81536c8 Let me know what you think! |
Thanks @shadeyg56 I'm on my way to KubeCon today and will take a look once I'm back. @PurpleWazard if you could take a look and see if it works on your side until then that would be great. Thanks! |
Unfortunately now it doesn't work for me at all, with or without config file, i.e: auto-cpufreq --live is broken
auto-cpufreq GUI is broken
auto-cpufreq --install is broken
Even installer remove reported errors
|
oh geez, I forgot to pull the merged changes from master and test with those on my system. |
Should work as intended now |
…c#672) * Add load from user config from in XDG_CONFIG_HOME if available This update introduces the flexibility to load the configuration file from multiple locations, prioritizing user preferences and system standards. Previously, the configuration was strictly read from a hardcoded system path (`/etc/auto-cpufreq.conf`). Now, the application first checks if the user has specified a configuration file path via command line arguments. If not, it looks for a configuration file in the user's config directory (`$XDG_CONFIG_HOME/auto-cpufreq/auto-cpufreq.conf`). If neither is found, it defaults to the original system-wide configuration file. This allows users to add their auto-cpufreq configuration to their dotfiles. * If --config is set but invalid, exit with error * Remove redundant empty string check on config file path * Remove duplicate isfile check for config path See also: AdnanHodzic#672 (comment) * Update configuration options in README See also: AdnanHodzic#672
Co-authored-by: Steven Braun <steven.braun.mz@gmail.com>
Updated with merge from #672 and fixed an issue with sudo (root user) not being able to access $HOME |
All looking perfect now, except one minor thing. While I don't have this problem for i.e auto-cpufreq --install like I did originally and
|
Fixed with 00463b3 |
Unfortunately I now have a problem where I can't install auto-cpufreq daemon:
|
Hm... I'm unable to reproduce this in a Debian container. Did you run |
Yup, I uninstalled it first and then:
This is happening on Ubuntu 24.04 with Python 3.12.3. Could it also be related with pyinotify and removal of |
Ah, yes I had been running on 3.10. I wasn't aware that would happen in 3.12. The In e3678c1, I swapped the normal |
Our of sheer curiosity, why did you end up opting to use Also maybe a good idea to propose your fork back to upstream?
It all works as expected now! This has been a great improvement and will be a big feature as part of upcoming release, hence needless to say thank you for your contribution! |
Yeah, when I was looking for the best way to monitor files the two solutions I found were watchdog and inotify. Watchdog seemed more complicated than we needed it for and I saw that inotify was specifically for Linux only and was a part of the kernel itself. I guess pyinotify seemed like the more "Linux" way to do it, which seemed good to me seeing as
I believe somebody already made a PR of what I did (we are not the only ones to run into this issue), but I don't expect it to be merged due to lack of activity on the repo. |
This PR reworks the config into its own class/file and introduces config reloads when the config file is changed, created, or deleted. This also introduces the utils folder to remove some clutter from
core.py
The file watching is done using the
pyinotify
package which I've added as a Nix dependency and Poetry dependency. All file watching is done in a separate thread so that the daemon isn't blockedRather than using the old
get_config()
function which relied on setting a static attribute to the function, this uses a new_Config()
class, but the config is actually accessed by importingconfig
from theconfig.py
file. This allows the config to be accessed from any file, basically as a singleton._Config()
should NEVER be accessed directlyThis change can be tested by running the daemon, running
auto-cpufreq --stats
, then editing your config file. You should see the changes take place live in the stats window without any sort of daemon reload.